Skip to content

Conversation

@NicholasTanz
Copy link
Contributor

Description of the changes being introduced by the pull request:

Fixes #2793

@NicholasTanz NicholasTanz requested a review from a team as a code owner February 20, 2025 02:54
@NicholasTanz NicholasTanz marked this pull request as draft February 20, 2025 02:54
@NicholasTanz NicholasTanz marked this pull request as ready for review February 20, 2025 03:07
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks.

  • left one change request: the release workflow is a bit tricky to test but I'm guessing the rc release name is not correct right now. This seems like the only merge blocking thing
  • On personas: I believe "auditor" persona is not appropriate but "pedantic" might be -- feel free to experiment if you want but be aware that some of the suggestions might require discussion. Keeping the default is totally fine for this PR though
  • The output is a little verbose maybe... Can you fiddle with verbosity settings to get the output to roughly match the other tools (in tox -e lint) -- one or two lines of output from zizmor would be ideal for successful run

@NicholasTanz
Copy link
Contributor Author

NicholasTanz commented Feb 20, 2025

  • On personas: I believe "auditor" persona is not appropriate but "pedantic" might be -- feel free to experiment if you want but be aware that some of the suggestions might require discussion. Keeping the default is totally fine for this PR though

switching to pedantic covered all of the suppressed results (5) and they seem fairly reasonable to me

4/5 of them were due to unpinned "uses", but those were all unpinned with the reason that security wasn't critical. I tried using the ignore with the trailing explanation but it didn't seem to work.

edit: My bad, the ignore with trailing explanation feature isn't out yet as of v1.3.1.

1/5 was due to excessive permissions and even though there's just one job in that workflow, I think it makes sense to specify the permissions in the job

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@jku jku merged commit 39388c3 into theupdateframework:develop Feb 21, 2025
17 checks passed
@NicholasTanz NicholasTanz deleted the addZizmor branch February 22, 2025 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lint workflows with zizmor

2 participants